roc_audio: adjust resampler coef #113

dshil · 2017-10-15T14:01:26Z

gavv · 2017-10-16T07:07:05Z

src/lib/roc/config.h

@@ -105,6 +105,9 @@ typedef struct roc_receiver_config {

    //! A bitmask of ROC_FLAG_* constants.
    unsigned int flags;
+
+    //! Number of samples for all channels per second.
+    unsigned int sample_rate;


This comment looks ambiguous. I suggest "Number of samples per second per channel". We should also fix corresponding comments in ReceiverConfig and SenderConfig.

gavv · 2017-10-16T07:08:28Z

src/modules/roc_audio/resampler_updater.cpp

    if (rate_limiter_.allow()) {
        roc_log(LogDebug, "resampler updater: queue_size=%lu fe=%.5f",
-                (unsigned long)queue_size, (double)fe_.freq_coeff());
+                (unsigned long)queue_size, (double)adjusted_coef);


It would be reasonable to print both freq_coeff and adjusted_coef here.

gavv · 2017-10-16T07:09:06Z

src/modules/roc_audio/resampler_updater.h

@@ -35,7 +35,8 @@ class ResamplerUpdater : public packet::IWriter,
    //!  - @p update_interval defines how often to call FreqEstimator, in samples
    //!  - @p aim_queue_size defines FreqEstimator target queue size, in samples
    ResamplerUpdater(packet::timestamp_t update_interval,
-                     packet::timestamp_t aim_queue_size);
+                     packet::timestamp_t aim_queue_size,
+                     const float sample_rate_coef);


sample_rate_coef parameter should be documented.

gavv · 2017-10-16T07:15:03Z

src/modules/roc_pipeline/config.h

@@ -135,7 +135,7 @@ struct ReceiverConfig {
    bool timing;

    ReceiverConfig()
-        : sample_rate(DefaultSampleRate)
+        : sample_rate(0)


Why this change? The idea of default constructors of these *Config structures is that if a field was not overwritten explicitly, it has a reasonable value. This feature will be used in roc_lib, which will overwrite only those fields that were set to non-zero by user.

Providing zero sample rate for sox means, that it'll determine sample rate of the output file or device by its own.

UPD: Later I've checked, that if we provide unsupported sample rate to sox (e.g. 44K, while device supports 48K), sox will also detect the correct sample rate by its own.

I'll rollback this change.

This feature will be used in roc_lib, which will overwrite only those fields that were set to non-zero by user.

Where do we check that the user provides non-zero values?

bool make_receiver_config(pipeline::ReceiverConfig& out, const roc_receiver_config* in) { out.default_session.latency = in->latency; out.default_session.timeout = in->timeout; out.default_session.samples_per_packet = in->samples_per_packet; out.sample_rate = in->sample_rate; //... }

We'll panic only during a session creation for zero sample rate.

Later I've checked, that if we provide unsupported sample rate to sox (e.g. 44K, while device supports 48K), sox will also detect the correct sample rate by its own.

I think we should not rely on this, it's too implicit and can be changed when switching from SoX to something else.

I suggest the following:

Use a reasonable default in ReceiverConfig::sample_rate (44100). This default will be ignored in roc_recv, but it will be used in roc_lib.

Use a separate variable in roc_recv, initialize it with --rate option value or zero if it wan't given, pass it to SoX, then get the actual sample rate from SoX, and write it to ReceiverConfig.

Where do we check that the user provides non-zero values?

There are no checks yet. However, I plan to add them in 0.1. It will look like:

if (in->sample_rate) { out.sample_rate = in->sample_rate; }

gavv · 2017-10-16T07:19:18Z

src/modules/roc_pipeline/receiver_session.cpp

@@ -35,7 +36,8 @@ ReceiverSession::ReceiverSession(const SessionConfig& config,

    if (config.resampling) {
        resampler_updater_.reset(new (allocator_) audio::ResamplerUpdater(
-                                     config.fe_update_interval, config.latency),
+                                     config.fe_update_interval, config.latency,
+                                     (float)format->sample_rate / out_sample_rate),


We should panic if out_sample_rate is zero. A panic is better than a segfault.

gavv · 2017-10-16T07:20:08Z

src/modules/roc_sndio/target_sox/roc_sndio/player.cpp

    return true;
 }

 void Player::stop() {
    stop_ = 1;
 }

+void Player::start(pipeline::IReceiver* input) {


Why a pointer? It's not allowed to be null, so a reference is preferred.

gavv · 2017-10-16T07:22:25Z

src/modules/roc_sndio/target_sox/roc_sndio/player.cpp

+    if (output_) {
+        return size_t(output_->signal.rate);
+    }
+    return 0;


If output_ is null, it means that open() was not called or failed, and the user called get_sample_rate() anyway. It looks like a bug in the user code, so I think it's better to panic here instead of silently returning zero.

gavv · 2017-10-16T07:23:26Z

src/modules/roc_sndio/target_sox/roc_sndio/player.cpp

@@ -115,7 +128,7 @@ void Player::loop_() {
    SOX_SAMPLE_LOCALS;

    while (!stop_) {
-        pipeline::IReceiver::Status status = input_.read(frame);
+        pipeline::IReceiver::Status status = input_->read(frame);


We should panic if run() was called and input_ is null. A panic is better than a segfault.

gavv · 2017-10-16T07:24:47Z

src/modules/roc_sndio/target_sox/roc_sndio/player.h

    //! Stop thread.
    //! @remarks
    //!  Can be called from any thread.
    void stop();

+    //! Get sample rate
+    //!


It's worth to mention how this sample rate is obtained.

Also, we need a test for this feature.

gavv · 2017-10-16T07:29:21Z

src/tools/roc_recv/main.cpp

+        return 1;
+    }
+
+    config.sample_rate = player.get_sample_rate();


I think we should check if sample_rate is zero and exit with error in this case.

dshil · 2017-10-20T06:48:26Z

ping @gavv

gavv · 2017-10-20T07:28:10Z

src/modules/roc_audio/resampler_updater.cpp

@@ -96,13 +98,16 @@ bool ResamplerUpdater::update(packet::timestamp_t time) {
        update_time_ += update_interval_;
    }

+    const float adjusted_coef = float(sample_rate_coef_ * fe_.freq_coeff());


Why do we need a cast?

o my gosh, at the beginning I had a crazy idea, that we can overflow here, but after your comment, I've reread the code. Both values are near about 1, we should throw this cast.

I don't get it. If both arguments are floats, this cast simply does nothing.

gavv · 2017-10-20T07:31:49Z

src/modules/roc_sndio/target_sox/roc_sndio/player.cpp

 void Player::run() {
    roc_log(LogDebug, "player: starting thread");

+    if (!input_) {
+        roc_panic("player: thread is started before device or output file was opened");


The panic message is a bit misleading, because input_ is set in start(), not in open().

gavv · 2017-10-20T07:32:44Z

src/modules/roc_sndio/target_sox/roc_sndio/player.h

@@ -60,11 +58,23 @@ class Player : public core::Thread {
    //!  Should be called once before calling start().
    bool open(const char* name, const char* type = NULL);

+    //! Start reading samples.


It's worth to mention that it starts a thread.

gavv · 2017-10-20T07:36:17Z

src/modules/roc_sndio/target_sox/roc_sndio/player.h

+    //! Get a user defined or auto-detected sample rate of an output file or a device.
+    //!
+    //! @remarks
+    //! Output file or device should be opened.


Two little notices:

We can change @remarks to @pre (preconditions) here, it would fit a bit better.

We usually indent @remarks, @pre, and similar sections, e.g.:

//! @pre //! Output file or device should be opened.

gavv · 2017-10-20T07:36:46Z

src/tests/roc_lib/test_sender_receiver.cpp

@@ -223,6 +224,7 @@ TEST_GROUP(sender_receiver) {
        receiver_conf.n_repair_packets = n_repair_packets;
        receiver_conf.latency = packet_len * 20;
        receiver_conf.timeout = packet_len * 300;
+        receiver_conf.sample_rate = pipeline::DefaultSampleRate;


Since we have the default back, this line can be removed.

gavv · 2017-10-20T07:42:33Z

src/tests/roc_sndio/target_sox/test_player_recorder.cpp

    player.join();
 }

+TEST(player_recorder, player_get_sample_rate) {


I think we should test three cases:

The user provided zero sample rate, SoX selected non-zero sample rate, Player was successfully opened.

The user provided non-zero sample rate, SoX selected the same sample rate, Player was successfully opened.

The user provided non-zero sample rate, but SoX selected some other sample rate. I think Player::open should fail in this case, because it's generally not a good idea to ignore what the user have asked explicitly.

I'm not sure if it's possible to add a test for the latter case because our tests operate with files instead of devices. However, we need to fix the behaviour anyway.

gavv · 2017-10-20T07:46:52Z

src/tools/roc_recv/main.cpp

@@ -148,6 +148,22 @@ int main(int argc, char** argv) {
    core::BufferPool<audio::sample_t> sample_buffer_pool(allocator, MaxFrameSize, 1);
    packet::PacketPool packet_pool(allocator, 1);

+    sndio::Player player(sample_buffer_pool, allocator, args.oneshot_flag,
+                         config.channels, config.sample_rate);


See #113 (comment)

gavv · 2017-10-21T10:38:26Z

src/modules/roc_audio/resampler_updater.h

@@ -34,8 +34,11 @@ class ResamplerUpdater : public packet::IWriter,
    //! @b Parameters
    //!  - @p update_interval defines how often to call FreqEstimator, in samples
    //!  - @p aim_queue_size defines FreqEstimator target queue size, in samples
+    //!  - @p sample_rate_coef represents an adjustment coefficient for a resampler
+    //!       scaling factor, calculating from sample rates of input and output devices.


from sample rates of input and output devices

Well, strictly speaking it's not rates of the input and output devices. It's a faсtor to convert session rate to receiver output rate.

gavv · 2017-10-21T10:42:25Z

src/modules/roc_sndio/target_sox/roc_sndio/player.cpp

+    size_t out_rate = size_t(output_->signal.rate);
+
+    if (in_rate != 0 && in_rate != out_rate) {
+        roc_log(LogError, "player: sample rates mismatch, auto-detected=%zu, input=%zu",


I think the error message should be something like:

"can't open output file or device with the required sample rate: required=%lu suggested=%lu".

Also some notes:

we don't use %z, it's not portable

we don't use commas when logging multiple values, they don't increase readability, but they make grepping a bit harder

gavv · 2017-10-21T10:43:31Z

src/tests/roc_lib/test_sender_receiver.cpp

@@ -22,6 +22,7 @@
 #include "roc_packet/address_to_str.h"
 #include "roc_packet/packet_pool.h"
 #include "roc_packet/parse_address.h"
+#include "roc_pipeline/config.h"


This line can be removed I guess.

gavv · 2017-10-21T10:59:33Z

src/tests/roc_sndio/target_sox/test_player_recorder.cpp

+
+TEST(player_recorder, player_open_sample_rates_mistmatch) {
+    Player player(buffer_pool, allocator, true, ChMask, SampleRate + 1);
+    CHECK(!player.open(NULL, NULL));


There are three problems with this test:

Opening an audio device may have side-effects. For example, if the user is using ALSA without dmix, only one process can open an audio device. So this test will either fail to open it (if it's already opened by someone else), or prevent other processes from opening it for a short time.

If this test will fail to open device for some other reason (e.g. an ALSA device is already opened by someone else), it will pass anyway, so we really don't known if it actually tests something.

I'm not sure that any SoX backend is guaranteed to fail with rate 44100+1.

We theoretically may want a separate set of tests that work with a real audio device. Such tests should not be run automatically, but instead only when the user explicitly asked that. However I'm not sure it's worth it.

Thus, if we can't cover this case with a test that uses a temporary file instead of real audio device, I suggest to remove it for now.

I think that you've reviewed an old version of tests, I've rebased recently, but I totally agree with 3 points.

The first two points are still relevant after rebase.

gavv requested changes Oct 16, 2017

View reviewed changes

gavv requested changes Oct 20, 2017

View reviewed changes

gavv reviewed Oct 21, 2017

View reviewed changes

roc_audio: adjust reseampler coef

587b3be

gavv approved these changes Oct 22, 2017

View reviewed changes

gavv merged commit bb2ffcd into roc-streaming:develop Oct 22, 2017

dshil mentioned this pull request Dec 19, 2017

ensure to enable resampling for different in and out sample rates #142

Merged

dshil deleted the dshil/85-sample-rate branch January 20, 2018 06:55

gavv modified the milestones: 0.1.1, 0.1.0 Jun 23, 2019

roc_audio: adjust resampler coef #113

roc_audio: adjust resampler coef #113

Conversation

dshil commented Oct 15, 2017 • edited by gavv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshil Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshil Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshil commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavv Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshil Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshil commented Oct 15, 2017 •

edited by gavv

Loading

dshil Oct 18, 2017 •

edited

Loading

dshil Oct 20, 2017 •

edited

Loading

gavv Oct 20, 2017 •

edited

Loading

dshil Oct 21, 2017 •

edited

Loading